[py][bidi]: enable download event tests for firefox by navin772 · Pull Request #16587 · SeleniumHQ/selenium

navin772 · 2025-11-12T16:45:12Z

User description

🔗 Related Issues

💥 What does this PR do?

Enables test_add_event_handler_download_end and test_add_event_handler_download_will_begin tests for Firefox.
From Firefox 145, these events are supported.

Also, makes the assertion generalized in case of existing downloads.

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

Cleanup (formatting, renaming)

PR Type

Tests

Description

Remove xfail markers for Firefox download event tests
Generalize filename assertion to handle existing downloads
Support Firefox 145+ download event functionality

Diagram Walkthrough

flowchart LR
  A["Firefox 145+ Support"] --> B["Remove xfail Markers"]
  B --> C["test_add_event_handler_download_will_begin"]
  B --> D["test_add_event_handler_download_end"]
  E["Generalize Assertion"] --> F["Handle Filename Variations"]
  F --> C

File Walkthrough

Relevant files

Tests

bidi_browsing_context_tests.py `Enable Firefox download event tests with generalized assertions` py/test/selenium/webdriver/common/bidi_browsing_context_tests.py Removed `@pytest.mark.xfail_firefox` decorator from `test_add_event_handler_download_will_begin` test Removed `@pytest.mark.xfail_firefox` decorator from `test_add_event_handler_download_end` test Updated filename assertion to use substring matching instead of exact equality Added comment explaining filename variation due to existing downloads	+2/-3

qodo-code-review · 2025-11-12T16:45:39Z

PR Compliance Guide 🔍

(Compliance updated until commit `5ca7c96`)

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: No audit logs: The new test code adds assertions and event handling for downloads without introducing or verifying any audit logging for critical actions, which may be acceptable for tests but cannot be confirmed from this diff. Referred Code assert len(events_received) == 1 # filename maybe file_1.txt or file_1(1).txt depending on existing files in download dir assert "file_1" in events_received[0].suggested_filename driver.browsing_context.remove_event_handler("download_will_begin", callback_id) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Limited edge handling: The generalized filename assertion only checks for "file_1" substring and relies on a 5s wait without explicit error handling or alternate outcomes, which may be acceptable in tests but lacks explicit edge-case handling in this diff. Referred Code WebDriverWait(driver, 5).until(lambda d: len(events_received) > 0) assert len(events_received) == 1 # filename maybe file_1.txt or file_1(1).txt depending on existing files in download dir assert "file_1" in events_received[0].suggested_filename Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Previous compliance checks

Compliance check up to commit 56e98cd

Security Compliance

⚪

Weak assertion

Description: The assertion on suggested_filename is loosened to a substring match ("file_1"), which
could allow false positives and mask regressions in download handling; consider validating
against a stricter expected pattern to avoid missing discrepancies.
bidi_browsing_context_tests.py [803-806]

Referred Code

assert len(events_received) == 1
# filename maybe file_1.txt or file_1(1).txt depending on existing files in download dir
assert "file_1" in events_received[0].suggested_filename

Ticket Compliance

🟡

🎫 #5678

🔴	I
	n
	v
	e
	s
	t
	i
	g
	a
	d
	r
	o
	l
	p
	"
	E
	:
	C
	c
	F
	u
	(
	f
	)
	w
	h
	m
	D
	U
	b
	1
	6
	.
	0
	4
	5
	2
	3
	S
	9
	P
	x
	q
⚪	-

🟡

🎫 #1234

🔴	E
	n
	s
	u
	r
	e
	W
	b
	D
	i
	v
	2
	.
	4
	8
	t
	g
	J
	a
	S
	c
	p
	h
	o
	'
	f
	l
	k
	(
	)
	F
	x
	w
	y
	d
	7
	1
	P
	m
⚪	-

Codebase Duplication Compliance

⚪

Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance

🟢

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

⚪

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
No audit logs: The new assertions and event handling in tests add no audit logging for critical actions,
but as test code this may be acceptable and outside production audit requirements.

Referred Code

assert len(events_received) == 1
# filename maybe file_1.txt or file_1(1).txt depending on existing files in download dir
assert "file_1" in events_received[0].suggested_filename

driver.browsing_context.remove_event_handler("download_will_begin", callback_id)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Minimal error handling: The test relies on assertions and a short wait without explicit handling for timing or
environment edge cases, which is typical for tests but may cause flakiness.

Referred Code

WebDriverWait(driver, 5).until(lambda d: len(events_received) > 0)

assert len(events_received) == 1
# filename maybe file_1.txt or file_1(1).txt depending on existing files in download dir
assert "file_1" in events_received[0].suggested_filename

Learn more about managing compliance generic rules or creating your own custom rules

qodo-code-review · 2025-11-12T16:46:48Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix race condition in test Add a short `time.sleep()` delay before asserting the number of events received to prevent a potential race condition and improve test robustness. py/test/selenium/webdriver/common/bidi_browsing_context_tests.py [799-805] download_xpath_file_1_txt = '//*[@id="file-1"]' driver.find_element(By.XPATH, download_xpath_file_1_txt).click() WebDriverWait(driver, 5).until(lambda d: len(events_received) > 0) + +# Allow a moment for any unexpected additional events to arrive. +time.sleep(0.2) assert len(events_received) == 1 # filename maybe file_1.txt or file_1(1).txt depending on existing files in download dir assert "file_1" in events_received[0].suggested_filename `[To ensure code accuracy, apply this suggestion manually]` Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies a potential race condition in an asynchronous test and proposes adding a short delay to improve its stability, which is a valid approach to prevent test flakiness.	Low
Update

enable tests for firefox

56e98cd

selenium-ci added the C-py Python Bindings label Nov 12, 2025

qodo-code-review bot added the Review effort 2/5 label Nov 12, 2025

cgoldberg approved these changes Nov 12, 2025

View reviewed changes

Merge branch 'trunk' into py-enable-download-event-tests-ff

5ca7c96

navin772 merged commit 55fa188 into SeleniumHQ:trunk Nov 13, 2025
22 checks passed

navin772 deleted the py-enable-download-event-tests-ff branch November 13, 2025 04:52

This was referenced Dec 15, 2025

Bump the stagezero-selenium group with 2 updates mitchHay/StageZero#263

Closed

Bump the functionaltests group with 4 updates samsmithnz/SamsFeatureFlags#1188

Closed

dependabot bot mentioned this pull request Dec 22, 2025

chore(deps): nuget update: Bump Selenium.WebDriver from 4.34.0 to 4.39.0 BySuspect/AutoPixAiCreditClaimer#40

Closed

dependabot bot mentioned this pull request Feb 11, 2026

chore: Bump Selenium.WebDriver from 4.36.0 to 4.40.0 appium/dotnet-client#1008

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[py][bidi]: enable download event tests for firefox#16587

[py][bidi]: enable download event tests for firefox#16587
navin772 merged 2 commits intoSeleniumHQ:trunkfrom
navin772:py-enable-download-event-tests-ff

navin772 commented Nov 12, 2025 •

edited by qodo-code-review bot

Loading

Uh oh!

qodo-code-review bot commented Nov 12, 2025 •

edited

Loading

Uh oh!

qodo-code-review bot commented Nov 12, 2025 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

navin772 commented Nov 12, 2025 • edited by qodo-code-review bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

🔗 Related Issues

💥 What does this PR do?

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

PR Type

Description

Diagram Walkthrough

File Walkthrough

Uh oh!

qodo-code-review bot commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Compliance Guide 🔍

(Compliance updated until commit 5ca7c96)

Previous compliance checks

Uh oh!

qodo-code-review bot commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

navin772 commented Nov 12, 2025 •

edited by qodo-code-review bot

Loading

qodo-code-review bot commented Nov 12, 2025 •

edited

Loading

(Compliance updated until commit `5ca7c96`)

qodo-code-review bot commented Nov 12, 2025 •

edited

Loading